Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize performance for binder, imports and packages #1702

Closed
wants to merge 4 commits into from

Conversation

sbalabanov
Copy link

This PR consists of multiple commits and is pure refactoring. It should maintain full backwards compatibility with the origin.

  • imports.goModuleRoot is called for every type resolution. It tries to find go.mod file in the directory tree making a gazillion of requests to open file. While someone more familiar with the application design can come up with a better architecture to avoid this altogether, I added a caching layer on top of this function to only traverse file system once.
  • binder.FindObject traverses all definitions in a package and for all needed types it is effectively O(n^2). Make it to O(n) by caching all package top-level definitions in a hashtable.
  • packages.Load calls 'packages.LoadAll' which allocates a slice from the heap even when a single package is required. Added a fast path by checking a cache map before calling to packages.LoadAll to avoid that. A possibly better solution is to refactor common parts of packages.Load and packages.LoadAll into a separate function.

For the targeted use case we achieved ~20% speed optimizations after those improvements.

Because functionality did not change, existing tests should cover it all so no new tests added.

@StevenACoffman
Copy link
Collaborator

@sbalabanov Thank you for your contribution! It looks like there are some linting and test failures. Can you make sure your files are formatted using gofmt (or gofumpt), and you've regenerated everything (including the examples) and ran (and passed) all the tests?

gofmt -s -w .
go generate ./... && cd example/ && go generate ./... && cd ..
go test -race ./... && cd example && go test -race ./... && cd ..

@sbalabanov
Copy link
Author

Can I please have an approval to run workflows on PRs? On the fork I am getting lint error which seems unrelated.
Test failures seem to be fixed now.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 67.655% when pulling 949233b on sbalabanov:master into 80713b8 on 99designs:master.

@StevenACoffman
Copy link
Collaborator

Merged in #1711

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants